Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Startup Profiling 1 - Decouple Profiler from Transaction #3101

Merged

Conversation

stefanosiano
Copy link
Member

@stefanosiano stefanosiano commented Dec 15, 2023

📜 Description

removed the need of a transaction to start a profile in AndroidTransactionProfiler, allowing to bind one later
added a constructor to AndroidTransactionProfiler without options
added options as param to AndroidTransactionProfiler.onTransactionFinish
some cleanup in the profiler

💡 Motivation and Context

For the startup profiling feature, we need to create a profiler instance before the SDK is initialized, which means we don't have the options and cannot start any transaction, yet.
This change allows to create an instance of the profiler with only the parameters needed, without passing the whole SentryOptions object, and also allows to run the profiler before the transaction, and bind a transaction later on.

💚 How did you test it?

Unit tests

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

…ctionProfiler, allowing to bind one later

some cleanup in the profiler
added options as param to AndroidTransactionProfiler.onTransactionFinish
Copy link
Contributor

github-actions bot commented Dec 15, 2023

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 95818bf

@stefanosiano
Copy link
Member Author

Possible breaking changes:
options.setTransactionProfiler now works only if no profiler was set, however this API was never meant to be used by the user
AndroidTransactionProfiler has now deprecated constructors, which could be removed completely if they are not used by Hybrid SDKs
AndroidTransactionProfiler methods (internal) have changed. This doesn't affect users, but can affect Hybrid SDKs.
@krystofwoldrich Can you confirm these are not actually breaking changes for hybrid SDKs?

Copy link
Contributor

github-actions bot commented Dec 15, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 393.92 ms 460.27 ms 66.35 ms
Size 1.70 MiB 2.27 MiB 583.85 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
b172d4e 412.60 ms 492.68 ms 80.08 ms
4e29063 384.14 ms 447.74 ms 63.60 ms
283d83e 348.44 ms 392.06 ms 43.62 ms
4e260b3 378.73 ms 454.18 ms 75.45 ms
c7e2fbc 393.98 ms 478.24 ms 84.27 ms
d6d2b2e 413.20 ms 486.76 ms 73.56 ms
93a76ca 378.48 ms 451.78 ms 73.30 ms
c3f503e 360.41 ms 434.67 ms 74.27 ms
1f3652d 367.21 ms 438.46 ms 71.25 ms
baaf637 375.71 ms 441.58 ms 65.87 ms

App size

Revision Plain With Sentry Diff
b172d4e 1.72 MiB 2.29 MiB 578.09 KiB
4e29063 1.72 MiB 2.29 MiB 578.38 KiB
283d83e 1.72 MiB 2.29 MiB 577.69 KiB
4e260b3 1.72 MiB 2.27 MiB 554.95 KiB
c7e2fbc 1.72 MiB 2.29 MiB 576.40 KiB
d6d2b2e 1.72 MiB 2.27 MiB 555.05 KiB
93a76ca 1.72 MiB 2.29 MiB 576.75 KiB
c3f503e 1.72 MiB 2.29 MiB 577.04 KiB
1f3652d 1.72 MiB 2.27 MiB 554.95 KiB
baaf637 1.72 MiB 2.27 MiB 558.42 KiB

Previous results on branch: feat/early-profiling1-decouple-profiler-transaction

Startup times

Revision Plain With Sentry Diff
e42754c 382.06 ms 465.49 ms 83.43 ms
604628a 389.84 ms 456.78 ms 66.94 ms
1da0e4d 375.46 ms 448.53 ms 73.07 ms

App size

Revision Plain With Sentry Diff
e42754c 1.72 MiB 2.27 MiB 557.10 KiB
604628a 1.72 MiB 2.27 MiB 558.57 KiB
1da0e4d 1.72 MiB 2.27 MiB 558.36 KiB

@stefanosiano stefanosiano changed the title Early profiling 1 - Decouple Profiler from Transaction Startup Profiling 1 - Decouple Profiler from Transaction Dec 19, 2023
Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Nice clean cut 🥷 Just left a minor nit

CHANGELOG.md Outdated Show resolved Hide resolved
* added TransactionContext.isForNextStartup
* added SentryOptions.enableStartupProfiling
* added SentryStartupProfilingOptions class with Json ser/deser
* added startupProfilingConfigFile deletion and creation on init
* added sampling decision on SDK init with isForNextStartup flag set to true
* added SentryOptions.getCacheDirPathWithoutDsn for startupProfiling config file

* Startup Profiling 3 - Add ContentProvider and start profile (#3128)
* first activity transaction now inherits startup sampling decision, if available
* if a startup profiler was instantiated, it will be reused in AndroidOptionsInitializer, instead of creating a new one
* added ITransactionProfiler.isRunning
* startup profiler and sampling decision is stored in AppStartMetrics
* startup profile is bound to the startup transaction
* added io.sentry.profiling.enable-startup manifest option
* moved profilingTracesHz from SentryAndroidOptions to SentryOptions
* added startup profiling launch in SentryPerformanceProvider
* added isStartupTransaction to TransactionOptions
@stefanosiano stefanosiano merged commit 5784ed8 into main Jan 15, 2024
20 checks passed
@stefanosiano stefanosiano deleted the feat/early-profiling1-decouple-profiler-transaction branch January 15, 2024 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants